-
Notifications
You must be signed in to change notification settings - Fork 23
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Write critical operations to the audit log #959
Conversation
Signed-off-by: Hoang Pham <hoangmaths96@gmail.com>
Signed-off-by: Hoang Pham <hoangmaths96@gmail.com>
Signed-off-by: Hoang Pham <hoangmaths96@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for this and sorry for taking so long to check. This looks very good to me already. The conceptual changes make a lot of sense to have events introduced for those actions and while we might not have a immediate use case for the service contract, this could become quite useful in the future.
I left a few minor comments, maybe you can check those. Looking forward to your updates/comments on that.
lib/Db/LegacyRowMapper.php
Outdated
/** | ||
* @param (int|string)[][] $sortArray | ||
* | ||
* @psalm-param list<array{columnId: int, mode: 'ASC'|'DESC'}> $sortArray |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't seem related to the implementation but fine to annotate here, just the static analysis fails due to the incomplete array description as the array gets enriched with a columnType key
Error: lib/Db/LegacyRowMapper.php:184:25: InvalidArrayOffset: Cannot access value on variable $sortRule using offset value of 'columnType', expecting 'columnId' or 'mode' (see https://psalm.dev/115)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@juliushaertl Thank you for spot on! Wondering why the ci/cd checks keeps failing (Not immediately but after few days)
Changed to this: @psalm-param list<array{columnId?: int, columnType?: string, mode?: 'ASC'|'DESC'}> $sortArray
Because when running psalm at my local there a case only 2 of them present (columnId & mode)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The CI requires approval for outside contributors, i just approved it again, so results should appear more quickly now.
I'm also unsure why that may not happen for you locally, just checked and I get the same error. The only thing I could imagine is psalm cache which sometimes messes things up. You could try running with composer run psalm -- --no-cache --threads=8
and see if that makes a difference.
Signed-off-by: Hoang Pham <hoangmaths96@gmail.com>
Hello there, We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process. Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6 Thank you for contributing to Nextcloud and we hope to hear from you soon! |
This PR introduces audit logging for critical operations in the Tables app, addressing issue #956. The goal is to enhance compliance measures by logging critical operations into a dedicated log file.
Changes
Introduced
AuditLogServiceInterface
, which outlines the contract for audit logging within the application. TheDefaultAuditLogService
is the default implementation that dispatchesCriticalActionPerformedEvent
to interact with the AuditLog App. This service is auto-resolved by registering it in the Dependency Injection ContainerApplication.php
. This design allows for future extensibility, such as integrating different logging methods or services for AuditLog.Developed event dispatchers for corresponding events to trigger audit logs (
TableDeletedEvent
=>WhenRowDeletedAuditLogListener
,ViewDeletedEvent
=>WhenViewDeletedAuditLogListener
, etc.). This is a standard event-driven approach that decouples auxiliary logic (logging, mail sending, etc.) from the core business logic, effectively handling side effects. Most of these operations can be executed asynchronously via a queue. Each Listener is encapsulated in its own class adhering to the Single Responsibility Principle. TheAuditLogServiceInterface
is injected into each listener to further promote decoupling.Testing
Unit tests have been added for the new service and listeners. These tests can be run by executing into the server container and running
composer test
.Potential Improvements
Implementing a separate background queue worker for all listeners could offload non-business logic from the main service logic, improving performance, especially when dealing with external API calls or IO intensive operations.
Applying Database Transactions: The TableService interacts with multiple tables simultaneously (Table, View, Row, Column). Implementing DB transactions and error handling mechanisms can ensure the integrity and reliability of operations.
Please review and provide your feedback, thank you @juliushaertl!